Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix spacing with VRow #395

Merged
merged 5 commits into from
Jan 9, 2021
Merged

Fix spacing with VRow #395

merged 5 commits into from
Jan 9, 2021

Conversation

robbevp
Copy link
Member

@robbevp robbevp commented Jan 5, 2021

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Fix #394

This negates the changes introduced by vuetifyjs/vuetify@d0f25fc#diff-40030629d72eca85d1eda4223b61cf497956726ca57bab2e64813010baf80cd8
The fix is done by

  • Adding a no-gutters prop, which sets all margins to 0
  • Manually setting vertical or bottom margins.
  • Wrapping the content of the row in VCol component

How Has This Been Tested?

  • I've tried to compare with our app before vuetify 2.4.0.
  • I went through all occurences of VRow to see where we needed to fix the margins

@robbevp robbevp added the bug Something isn't working label Jan 5, 2021
@robbevp robbevp added this to the 1.0 milestone Jan 5, 2021
@robbevp robbevp requested a review from chvp January 5, 2021 15:32
@robbevp robbevp self-assigned this Jan 5, 2021
chvp
chvp reviewed Jan 9, 2021
View reviewed changes
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this is still being worked on on the vuetify side: vuetifyjs/vuetify#12915 vuetifyjs/vuetify#12916
Are we running into the same thing? If so, I would just wait on the vuetify fix. (Adding no-gutters doesn't really make sense to add spacing IMO.)

@robbevp
Copy link
Member Author

robbevp commented Jan 9, 2021 •
edited
Loading

Seems like this is still being worked on on the vuetify side: vuetifyjs/vuetify#12915 vuetifyjs/vuetify#12916
Are we running into the same thing? If so, I would just wait on the vuetify fix. (Adding no-gutters doesn't really make sense to add spacing IMO.)

I don't think that issue/PR applies, since it is about VRow dense, which we don't use.
If you look at vuetifyjs/vuetify#12848 - it seems to me that Vuetify doesn't think this is an issue but rather intended. I would not wait on the off chance they change this behaviour.

no-gutters to fix can be confusing in the future. Changed to a my-0 utility class

chvp
chvp requested changes Jan 9, 2021
View reviewed changes
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't really get how this can be the intended behaviour by vuetify 🤷‍♀️, but I guess they're pretty set on it.

src/components/AlbumForm.vue Outdated
@@ -201,7 +201,7 @@
v-model="clear_review_comment"
:label="$tc('music.flag.clear', 1)"
/>
<VRow justify="center">
<VRow justify="center my-o">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o?

@chvp chvp merged commit 3531ce8 into develop Jan 9, 2021
@chvp chvp deleted the fix/spacing-row branch January 9, 2021 13:56
@robbevp robbevp mentioned this pull request Jan 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chvp chvp chvp approved these changes

Assignees

@robbevp robbevp

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong vertical spacing on various pages/components
2 participants
@robbevp @chvp